-
Couldn't load subscription status.
- Fork 5
Add ID wrapper classes #122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Leandro Lucarella <[email protected]>
The symbol in `typing` is deprecated. Signed-off-by: Leandro Lucarella <[email protected]>
|
This is to minimize the changes between the v0.15 and v0.17 clients, so it is easier to maintain a v0.17 branch in the SDK, as this change is very intrusive and could make merging new changes to a branch with this pretty conflict-prone. |
Signed-off-by: Leandro Lucarella <[email protected]>
Introduce `MicrogridId` and `ComponentId` classes to replace plain integer IDs. These classes provide type safety and prevent accidental errors by: - Making it impossible to mix up microgrid and component IDs (equality comparisons between different ID types always return false). - Preventing accidental math operations on IDs. - Providing clear string representations for debugging (MID42, CID42). - Ensuring proper hash behavior in collections. Signed-off-by: Leandro Lucarella <[email protected]>
Now the client uses the new strongly typed IDs for components and microgrids. Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an optional comment.
| # This is not an unidiomatic typecheck, that's an odd name for the check. | ||
| # isinstance() returns True for subclasses, which is not what we want here. | ||
| # pylint: disable-next=unidiomatic-typecheck | ||
| return type(other) is MicrogridId and self._id == other._id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can still do isinstance and disable inheritance for the class using the typing.final decorator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, not sure if typing.final also performs a runtime check or not, if it doesn't, then the current code is safer as it will be checked at runtime. In practice I don't think it should be a big deal anyway, I doubt anyone would want to subclass these IDs, but in any case adding typing.final() might be nice addition to make the intention clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.python.org/3.11/library/typing.html#typing.final
There is no runtime checking of these properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduce
MicrogridIdandComponentIdclasses to replace plain integer IDs. These classes provide type safety and prevent accidental errors by: